Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sourceInputCompletionHook: init #104225

Closed

Conversation

jonringer
Copy link
Contributor

@jonringer jonringer commented Nov 18, 2020

Motivation for this change

It's been a recurrent theme that there's some pain with nix-shell and bringing in some tools, only to have shell completion not work without adding additional code to shellHook. This solves that! (at least for bash)

This hasn't been tested with zsh or fish, as running nix-shell even with those shells will drop me into a bash shell.

If this does get added, may have it added to mkShell by default, as that's likely intended user behavior.

Also, I don't feel strongly about the name, if anyone can come up with a better name, I'll gladly change it.

example scenario:

$ kubectl
The program ‘kubectl’ is currently not installed. It is provided by
several packages. You can install it by typing one of the following:
  nix-env -iA nixos.kubectl
  nix-env -iA nixos.kubernetes
  nix-env -iA nixos.openshift
$ cat shell.nix
with import ./. { };

mkShell rec {
  buildInputs = [
    exa
    kubectl
    hyperfine
    sourceInputCompletionHook
  ];
}

$ nix-shell
Completions added for: kubectl hyperfine exa

[nix-shell:/home/jon/projects/nixpkgs]$ kubectl <tab><tab>
alpha          auth           convert        diff           get            plugin         scale          wait
annotate       autoscale      cordon         drain          kustomize      port-forward   set
api-resources  certificate    cp             edit           label          proxy          taint
api-versions   cluster-info   create         exec           logs           replace        top
apply          completion     delete         explain        options        rollout        uncordon
attach         config         describe       expose         patch          run            version
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@zowoq
Copy link
Contributor

zowoq commented Nov 18, 2020

Looks like #103501 is related?

@jonringer
Copy link
Contributor Author

jonringer commented Nov 18, 2020

definitely looks related, but need to verify if the completions actually work. For stdenv, there's really no need to add shell completion, so I would be suprised if it was sourcing shell completion for normal nix builds.

@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Nov 19, 2020
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/setup-bash-completion-when-running-under-nix-shell-on-macosx/5216/6

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/shell-nix-load-completions/8803/15

@jonringer
Copy link
Contributor Author

One thing of note, is that the other PR affects how XDG_DATA_DIRS works fundamentally with stdenv. While this only adds new behavior when included. (and I'll open another PR to add this to mkShell, if we feel like going in this direction)

@jonringer
Copy link
Contributor Author

cc @FRidh @zimbatm on opinions

@jonringer
Copy link
Contributor Author

jonringer commented Nov 19, 2020

I will have to revisit this for fish, as it's not compatible syntax

and zsh doesn't like my associative arrays

@@ -161,6 +161,8 @@ in
deps = [ innoextract file-rename ]; }
../build-support/setup-hooks/gog-unpack.sh;

sourceInputCompletionHook = callPackage ../shells/hooks/sourceInputCompletionHook { };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

explicitly state ShellHook?

Comment on lines +20 to +21
exit 0
esac
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
exit 0
esac
exit 0
;;
esac

@SuperSandro2000
Copy link
Member

Also, I don't feel strongly about the name, if anyone can come up with a better name, I'll gladly change it.

What about sourceShellCompletionHook?

local completionDir=$input/$completionPath
if [ -d $completionDir ]; then
for script in $(@findutils@/bin/find $completionDir -type f); do
. $script
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bash completions should be loaded lazily. This can be achieved with XDG_DATA_DIRS via #103501 or if that's too radical, a shell hook.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I'm thinking of closing this in favor of #103501 . Made this PR before I was aware of it's existence

@jonringer
Copy link
Contributor Author

jonringer commented Nov 19, 2020

I'm likely to close this in favor of #103501 . The other PR encourages better behavior by having individuals listing their dependencies in the appropriate nativeBuildInputs attr. I will instead just try to update example to use nativeBuildInputs instead of buildInputs

@jonringer
Copy link
Contributor Author

on master:

$ nix-shell -E 'with import ./. {}; mkShell { nativeBuildInputs = [ kubectl ]; }'
these paths will be fetched (7.96 MiB download, 41.76 MiB unpacked):
  /nix/store/hzpna3nl2pq784kxj6582jcbhj6ja12j-kubectl-1.19.5
copying path '/nix/store/hzpna3nl2pq784kxj6582jcbhj6ja12j-kubectl-1.19.5' from 'https://cache.nixos.org'...

[nix-shell:~/projects/nixpkgs]$ kubectl
alpha          auth           convert        diff           get            plugin         scale          wait
annotate       autoscale      cordon         drain          kustomize      port-forward   set
api-resources  certificate    cp             edit           label          proxy          taint
api-versions   cluster-info   create         exec           logs           replace        top
apply          completion     delete         explain        options        rollout        uncordon
attach         config         describe       expose         patch          run            version

@jonringer jonringer closed this Jan 11, 2021
@jonringer
Copy link
Contributor Author

I may open this back up again, as fish and zsh still don't have a good way to load completions.

@adrian-gierakowski
Copy link
Contributor

@jonringer yes please!

@adrian-gierakowski
Copy link
Contributor

Anything I can do to help revive this?

@roberth
Copy link
Member

roberth commented May 18, 2022

The existing and lazy solution already works for bash and fish. We can do

let pkgs = import ~/nixpkgs {}; in
pkgs.mkShell {
  packages = [ pkgs.hci pkgs.fish pkgs.zsh ];
}

and then nix-shell, use bash or invoke fish and voilà.

Zsh doesn't seem to work, but that could be for any reason, considering how sensitive scripts tend to be to their broader environment. I'm not a zsh user, so that doesn't help either.

I think the goal should be to get this to work in zsh, without introducing a worse architecture just for one shell.

@adrian-gierakowski
Copy link
Contributor

Thanks @roberth! I’ll try this out.

@sukhmel
Copy link

sukhmel commented Jun 15, 2023

I came upon this PR searching for some way to troubleshoot what's going on with completions in my shells, namely bash and zsh.

What I get right now is XDG_BASE_DIRS being correctly populated with /share contents, but bash refusing to load that completions at all. With zsh it seems like I'd need to also reload completions somehow, could you hint if it is possible already and what is the current state of affairs with this PR?

@jonringer jonringer deleted the add-source-input-completion-hook branch September 28, 2024 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants